Implement LowLevelCall library #5094
Conversation
🦋 Changeset detectedLatest commit: 0a80296 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
I did a lot of changes following yesterday's discussion. Only thing I havent done yet is polishing the contracts/utils/LowLevelCall.sol natspecs.
@ernestognw Let me know what you think !
|
|
Current remarquable numbers:
|
| assembly ("memory-safe") { | ||
| let fmp := mload(0x40) | ||
| returndatacopy(fmp, 0, returndatasize()) | ||
| revert(fmp, returndatasize()) |
There was a problem hiding this comment.
Should we increase/update free memory pointer before reverting the error (which I would call revert(result, returndatasize()) in my comment) in case the revert is caught by the client and execution flow continues?
There was a problem hiding this comment.
the revert data goes into the returndata (just like a return). It doesn't stay in memory, so no need to allocate it.
There was a problem hiding this comment.
Basically
- the called contract either does a return (success) or revert (failure) with some memory location (in its context)
- in both cases, the result is copied (by the evm) to "returndata"
- we go back to the context of the caller contract, they receive a boolean (success/faillure) and access to the returndata.
Note that when you do a call, you chagne the execution context, and that means you are in a "new memory space". This makes sure that the called contract cannot interfer with the stack and memory of the caller.
I invite you to run the following code on remix:
contract T1 {
function getFMP() public pure returns (uint256 fmp) {
assembly ("memory-safe") {
fmp := mload(0x40)
}
}
function allocateAndGetFMP(uint256 size) public pure returns (uint256 fmp) {
bytes memory buffer = new bytes(size);
buffer;
return getFMP(); // this cannot be catched (local jump)
}
function allocateAndCallGetFMP(uint256 size) public view returns (uint256 fmp) {
bytes memory buffer = new bytes(size);
buffer;
return this.getFMP(); // this is an EVM call that can be catched.
}
}| success = target.callNoReturn(data); | ||
|
|
||
| // Extract single 32-byte value | ||
| (success, result1, ) = target.callReturn64Bytes(data); |
There was a problem hiding this comment.
I like it that way tbh, just to make sure we wouldn't want to propose the following in addition to this callReturn64Bytes(data):
(success, result) = target.callReturnFirst32Bytes(data);(success, result) = target.callReturnSecond32Bytes(data);
There was a problem hiding this comment.
I think we should avoid having to many variants :
- reduced discoverability for user
- more code to maintain for us
| @@ -5,6 +5,9 @@ const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic'); | |||
|
|
|||
| const coder = ethers.AbiCoder.defaultAbiCoder(); | |||
|
|
|||
| const fakeContract = { interface: ethers.Interface.from(['error SomeCustomErrorWithoutArgs()']) }; | |||
| const returndata = fakeContract.interface.encodeErrorResult('SomeCustomErrorWithoutArgs'); | |||
There was a problem hiding this comment.
For user "convenience", would it be a shame to add an almost no-op function in the original lib contract which only reverts the error? (not a big fan though)
There was a problem hiding this comment.
I tried that, but unfortunatelly it does work like that.
Lets assume we have this library
library Lib {
error SomeError();
function selector() internal pure returns (bytes4) { return SomeError.selector;}
function trigger() internal pure { revert SomeError(); }
}and the following contracts:
contract Test1 {
function selector() public pure returns (bytes4) {
return Lib.selector();
}
}
contract Test2 is Test1 {
function trigger() public pure {
Lib.trigger();
}
}The compiler will include the error definition in Test2 (because the error is reachable in Test2.trigger -> Lib.trigger).
The compiler will NOT include the error definition in Test1, because it doesn't see how the error could be triggered by calling Test1 functions.
So if we were to make the error part of the contract, we need the user to use that no-op function somewhere in there contract.
Note that if we change the visibility of Test2.trigger to internal or private, the error is no longer part of the ABI.
| await expect(tx).to.changeEtherBalances([this.mock, this.target], [0n, 0n]); | ||
| await expect(tx) | ||
| .to.emit(this.mock, 'return$callReturn64Bytes_address_uint256_bytes') | ||
| .withArgs(false, ethers.ZeroHash, ethers.ZeroHash); |
There was a problem hiding this comment.
Should we add tests where success is false but result1&2 are not empty?
| } | ||
| } | ||
| return (false, 0); | ||
| Memory.setFreeMemoryPointer(ptr); |
There was a problem hiding this comment.
Why do we take care of fmp after this low level call but not really after others low level calls?
There was a problem hiding this comment.
Not sure about the other places, but in this particular case, we do it to dealocate the memory that was allocated by the abi.encodeCall(IERC20Metadata.decimals, ())
WalkthroughAdds a new LowLevelCall Solidity library and updates multiple callers to use it for low-level call/staticcall/delegatecall behavior and revert bubbling; updates Address/Create2/ERC4626 logic, expands mocks and tests, updates docs, and adds a changeset. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Lib as LowLevelCall
participant Target
Caller->>Lib: callNoReturn/staticcallNoReturn/delegatecallNoReturn(target, data[, value])
Lib->>Target: low-level call
alt success
Target-->>Lib: returndata (may be empty)
Lib-->>Caller: success=true
opt Caller requests data
Caller->>Lib: returnData()
Lib-->>Caller: bytes returndata
end
else failure
Target-->>Lib: revert data (may be empty)
alt has revert data
Lib--x Caller: bubbleRevert() (revert with returndata)
else no data
Lib--x Caller: revert FailedCall()
end
end
sequenceDiagram
autonumber
participant ERC as ERC4626
participant Lib as LowLevelCall
participant Asset as ERC20
ERC->>Lib: staticcallReturn64Bytes(Asset, encoded decimals())
Lib->>Asset: staticcall
alt success and >=32 bytes
Asset-->>Lib: returndata (>=32B)
Lib-->>ERC: (ok=true, word1, word2)
ERC->>ERC: decode uint8 if fits
else short/failed
Asset-->>Lib: short/empty or revert
Lib-->>ERC: (ok=false, 0, 0)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
contracts/token/ERC20/extensions/ERC4626.sol (1)
89-99: Safe, return-bomb–resistant decimals retrieval; fmp restore rationale acknowledged.
- staticcallReturn64Bytes + returnDataSize guard avoids copying large returndata.
- uint8 bound check prevents misbehaving tokens from inflating decimals.
- Restoring fmp only to deallocate abi.encodeCall buffer is intentional and consistent with prior discussion.
If you want to micro-opt for readability, cache returnDataSize once:
- return - (success && LowLevelCall.returnDataSize() >= 32 && uint256(returnedDecimals) <= type(uint8).max) + uint256 _rds = LowLevelCall.returnDataSize(); + return + (success && _rds >= 32 && uint256(returnedDecimals) <= type(uint8).max) ? (true, uint8(uint256(returnedDecimals))) : (false, 0);test/utils/LowLevelCall.test.js (3)
93-104: Don’t assume scratch words are zero on failure (insufficient balance).Per library docs, result1/result2 are undefined when success is false. Make the expectation robust.
Apply:
- await expect(tx) - .to.emit(this.mock, 'return$callReturn64Bytes_address_uint256_bytes') - .withArgs(false, ethers.ZeroHash, ethers.ZeroHash); + await expect(tx) + .to.emit(this.mock, 'return$callReturn64Bytes_address_uint256_bytes') + .withArgs(false, anyValue, anyValue);
105-114: Same: avoid asserting zeros on revert-without-reason.The two 32-byte words are not guaranteed to be zero on failure.
Apply:
- ) - .to.emit(this.mock, 'return$callReturn64Bytes_address_bytes') - .withArgs(false, ethers.ZeroHash, ethers.ZeroHash); + ) + .to.emit(this.mock, 'return$callReturn64Bytes_address_bytes') + .withArgs(false, anyValue, anyValue);
65-77: Add one test showing “false + non-empty results” to document scratch behavior.Proves why callers must not trust result words when success is false.
Apply (example placement after the first “with 64 bytes” happy-path test):
+ it('documents that result words may be stale when the call fails', async function () { + // Prime scratch with known values via a successful call + await this.mock.$callReturn64Bytes( + this.target, + this.target.interface.encodeFunctionData('mockFunctionWithArgsReturn', [returnValue1, returnValue2]), + ); + // Now force a failing call; success is false and words are unspecified (often the stale ones) + const tx = this.mock.$callReturn64Bytes( + this.target, + this.target.interface.encodeFunctionData('mockFunctionRevertsNoReason'), + ); + await expect(tx) + .to.emit(this.mock, 'return$callReturn64Bytes_address_bytes') + .withArgs(false, anyValue, anyValue); + });Also applies to: 116-133, 155-163
contracts/utils/Address.sol (1)
83-93: Branching is correct; consider micro-optimizing order only if it wins gas.Current structure optimizes the happy path readability. If CI shows wins, you could reorder the last two branches as previously suggested; otherwise keep as-is.
Possible alternative:
- } else if (LowLevelCall.returnDataSize() > 0) { - LowLevelCall.bubbleRevert(); - } else { - revert Errors.FailedCall(); - } + } else if (LowLevelCall.returnDataSize() > 0) { + LowLevelCall.bubbleRevert(); + } else { + revert Errors.FailedCall(); + }
🧹 Nitpick comments (11)
test/utils/Address.test.js (1)
280-282: Avoid chai-as-promised dependency; use resolved values insteadUse resolved values for equality checks to keep style consistent with the rest of the file and avoid requiring
eventually.Apply this diff:
- await expect(this.mock.$verifyCallResult(true, returndata)).to.eventually.equal(returndata); + expect(await this.mock.$verifyCallResult(true, returndata)).to.equal(returndata); @@ - await expect(this.mock.$verifyCallResultFromTarget(this.mock, true, returndata)).to.eventually.equal(returndata); - await expect(this.mock.$verifyCallResultFromTarget(this.recipient, true, returndata)).to.eventually.equal( - returndata, - ); + expect(await this.mock.$verifyCallResultFromTarget(this.mock, true, returndata)).to.equal(returndata); + expect(await this.mock.$verifyCallResultFromTarget(this.recipient, true, returndata)).to.equal(returndata); @@ - await expect(this.mock.$verifyCallResultFromTarget(this.mock, true, '0x')).to.eventually.equal('0x'); + expect(await this.mock.$verifyCallResultFromTarget(this.mock, true, '0x')).to.equal('0x');Also applies to: 297-301, 304-308
contracts/account/extensions/draft-AccountERC7579.sol (1)
318-322: Correct revert bubbling and returndata preservation in fallback.
- Uses call (per ERC-7579 requirement) and appends msg.sender per ERC-2771 format.
- On success, returns exact returndata; on failure, bubbles revert cleanly.
Optional: to avoid permanent fmp growth from abi.encodePacked in hot paths, snapshot and restore fmp around the call.
- if (LowLevelCall.callNoReturn(handler, msg.value, abi.encodePacked(msg.data, msg.sender))) { - return LowLevelCall.returnData(); - } else { - LowLevelCall.bubbleRevert(); - } + // Optional: bound memory growth from abi.encodePacked + if (true) { + bool ok; + bytes memory ret; + { + // snapshot fmp + Memory.Pointer _ptr = Memory.getFreeMemoryPointer(); + ok = LowLevelCall.callNoReturn(handler, msg.value, abi.encodePacked(msg.data, msg.sender)); + // restore fmp (returndatasize is unaffected) + Memory.setFreeMemoryPointer(_ptr); + } + if (ok) { + return LowLevelCall.returnData(); + } else { + LowLevelCall.bubbleRevert(); + } + }Add this import if you take the optional path:
import {Memory} from "../../utils/Memory.sol";test/utils/LowLevelCall.test.js (4)
1-4: Import anyValue to avoid brittle assertions on scratch results.Use anyValue for event args where result1/result2 are unspecified on failure; don't assert zeros.
Apply:
const { ethers } = require('hardhat'); const { expect } = require('chai'); -const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { anyValue } = require('@nomicfoundation/hardhat-chai-matchers/withArgs');
165-172: Static 64B: compare only status on failure.Deep-equality with zeroed words is brittle; assert just the boolean.
Apply:
- await expect( - this.mock.$staticcallReturn64Bytes( - this.target, - this.target.interface.encodeFunctionData('mockFunctionRevertsNoReason'), - ), - ).to.eventually.deep.equal([false, ethers.ZeroHash, ethers.ZeroHash]); + const [ok] = await this.mock.$staticcallReturn64Bytes( + this.target, + this.target.interface.encodeFunctionData('mockFunctionRevertsNoReason'), + ); + expect(ok).to.equal(false);
245-254: Delegate 64B: avoid zero assumptions on failure.Align with library contract; values are unspecified when success is false.
Apply:
- ) - .to.emit(this.mock, 'return$delegatecallReturn64Bytes') - .withArgs(false, ethers.ZeroHash, ethers.ZeroHash); + ) + .to.emit(this.mock, 'return$delegatecallReturn64Bytes') + .withArgs(false, anyValue, anyValue);
223-243: Add delegatecall revert-with-reason (64B split) test for parity with call/staticcall.Covers bubbling of first 64 bytes for delegatecall too.
Apply:
+ it('returns first 64 bytes of revert data on delegatecall revert with reason', async function () { + const encoded = ethers.Interface.from(['error Error(string)']).encodeErrorResult('Error', [ + 'CallReceiverMock: reverting', + ]); + await expect( + this.mock.$delegatecallReturn64Bytes( + this.target, + this.target.interface.encodeFunctionData('mockFunctionRevertsReason'), + ), + ) + .to.emit(this.mock, 'return$delegatecallReturn64Bytes') + .withArgs( + false, + ethers.hexlify(ethers.getBytes(encoded).slice(0x00, 0x20)), + ethers.hexlify(ethers.getBytes(encoded).slice(0x20, 0x40)), + ); + });contracts/utils/Address.sol (2)
116-127: DelegateCall: “success but no code” branch is effectively unreachable.DELEGATECALL to a non-contract can’t succeed. Keeping the branch is harmless but slightly noisy.
Apply (optional):
- } else if (success) { - revert AddressEmptyCode(target); } else if (LowLevelCall.returnDataSize() > 0) {
134-134: Typo in deprecation notice.“may be remove” → “may be removed”.
Apply:
- * NOTE: This function is DEPRECATED and may be remove in the next major release. + * NOTE: This function is DEPRECATED and may be removed in the next major release.contracts/utils/LowLevelCall.sol (3)
24-33: Docs: clarify success=false semantics and success=true with short return.You note results are undefined when success is false. Also mention that when success is true, result1 is valid iff returndatasize() >= 32, and result2 iff >= 64. Recommend callers check returnDataSize().
Apply:
- /// @dev Performs a Solidity function call using a low level `call` and returns the first 64 bytes of the result - /// in the scratch space of memory. Useful for functions that return a tuple of single-word values. + /// @dev Performs a Solidity function call using a low level `call` and returns the first 64 bytes of the result + /// in the scratch space of memory. Useful for functions that return a tuple of single-word values. + /// When success is true, result1 is meaningful iff returndatasize() >= 32, and result2 iff >= 64.
36-44: Doc reference points to old name.“Same as {callReturnBytes32Pair}” should reference callReturn64Bytes.
Apply:
- /// @dev Same as {callReturnBytes32Pair}, but allows to specify the value to be sent in the call. + /// @dev Same as {callReturn64Bytes}, but allows to specify the value to be sent in the call.
24-47: Optional: provide a “...ReturnScratch” API that also returns returndatasize.Returning size alongside the two words avoids a separate returnDataSize() read and helps users validate partial returns without another call.
For example:
+ function callReturnScratch(address target, uint256 value, bytes memory data) + internal + returns (bool success, uint256 size, bytes32 result1, bytes32 result2) + { + assembly ("memory-safe") { + success := call(gas(), target, value, add(data, 0x20), mload(data), 0, 0x40) + size := returndatasize() + result1 := mload(0x00) + result2 := mload(0x20) + } + }(Same pattern for staticcall/delegatecall.)
Also applies to: 56-70, 79-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.changeset/sharp-scissors-drum.md(1 hunks)contracts/account/Account.sol(2 hunks)contracts/account/extensions/draft-AccountERC7579.sol(2 hunks)contracts/mocks/CallReceiverMock.sol(2 hunks)contracts/mocks/Stateless.sol(1 hunks)contracts/token/ERC20/extensions/ERC4626.sol(2 hunks)contracts/utils/Address.sol(4 hunks)contracts/utils/Create2.sol(2 hunks)contracts/utils/LowLevelCall.sol(1 hunks)contracts/utils/README.adoc(2 hunks)docs/modules/ROOT/pages/utilities.adoc(1 hunks)test/utils/Address.test.js(2 hunks)test/utils/LowLevelCall.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
PR: OpenZeppelin/openzeppelin-contracts#5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.
Applied to files:
contracts/mocks/CallReceiverMock.sol
🧬 Code graph analysis (1)
test/utils/LowLevelCall.test.js (1)
test/utils/Memory.test.js (3)
require(1-1)require(2-2)require(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: tests
- GitHub Check: slither
- GitHub Check: tests-foundry
- GitHub Check: halmos
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (23)
.changeset/sharp-scissors-drum.md (1)
2-5: Changeset scope/readability look goodMinor bump with a concise summary of LowLevelCall is appropriate. No action needed.
contracts/utils/README.adoc (2)
42-44: Add LowLevelCall + Memory entries — LGTMConsistent with new library and placement next to Memory. Anchors read well.
142-145: Libraries index updated — LGTMEntries match the section above and preserve ordering.
test/utils/Address.test.js (3)
8-10: Precomputed custom-error returndata setup — LGTMClean way to drive bubbling assertions without deploying an extra contract.
284-293: Behavioral expectations — LGTMCustom error bubbling and empty-returndata defaulting to
FailedCallalign with Address.verifyCallResult* semantics.Also applies to: 321-329
311-319: Fix assertion: missing.bebeforerevertedWithCustomErrorThe matcher is
to.be.revertedWithCustomError(...). Withoutbethis will fail.Apply this diff:
- await expect(this.mock.$verifyCallResultFromTarget(this.mock, false, returndata)).to.revertedWithCustomError( + await expect(this.mock.$verifyCallResultFromTarget(this.mock, false, returndata)).to.be.revertedWithCustomError( fakeContract, 'SomeCustomErrorWithoutArgs', ); - await expect(this.mock.$verifyCallResultFromTarget(this.recipient, false, returndata)).to.revertedWithCustomError( + await expect(this.mock.$verifyCallResultFromTarget(this.recipient, false, returndata)).to.be.revertedWithCustomError( fakeContract, 'SomeCustomErrorWithoutArgs', );Likely an incorrect or invalid review comment.
contracts/mocks/CallReceiverMock.sol (2)
17-21: Memory-safe storage writes — LGTMUse of
assembly ("memory-safe") { sstore(...) }is appropriate for mocks exercising delegatecall and returndata paths.Also applies to: 28-33, 46-57
41-44: Return-shape variants — LGTMAdding tuple-return and pure/static variants provides good coverage for LowLevelCall tests.
Also applies to: 69-71
contracts/utils/Create2.sol (2)
7-7: Import LowLevelCall — LGTMScoped import keeps utils self-contained.
49-53: Bubble revert data on CREATE2 failure; fall back to FailedDeploymentThe control flow correctly mirrors EVM behavior: if constructor reverted with data, bubble it; otherwise revert with a canonical error. Nice improvement in diagnosability.
contracts/account/Account.sol (2)
9-9: Good move: centralize low-level calls via LowLevelCall.Keeps behavior consistent while standardizing revert/returndata handling across the codebase.
117-118: Prefund call keeps prior semantics (ignore success); confirm this is intentional for ERC-4337 flow.LowLevelCall.callNoReturn returns a bool that’s discarded here, matching the previous pattern. Please confirm no callers rely on a revert to detect prefund failure, since the entry point is expected to validate receipt.
contracts/mocks/Stateless.sol (1)
36-39: Import reorg LGTM.Single Memory import and new LowLevelCall import are consistent with the rest of the PR.
contracts/account/extensions/draft-AccountERC7579.sol (1)
20-20: Import LGTM.Aligns this extension with the shared LowLevelCall helper.
docs/modules/ROOT/pages/utilities.adoc (1)
470-488: Docs sample is clear; shows both ignore and decode paths.Good coverage of callNoReturn and callReturn64Bytes usage.
contracts/token/ERC20/extensions/ERC4626.sol (1)
9-10: Imports LGTM.Matches the new low-level call + memory management pattern.
contracts/utils/Address.sol (3)
38-45: sendValue: control-flow is clear; nice revert bubbling.Happy path returns early; on failure, bubbles or reverts with FailedCall. LGTM.
141-152: verifyCallResultFromTarget: behavior matches updated Address helpers.Checks “contract or data” on success and bubbles on failure. Looks correct.
158-166: verifyCallResult: minimal and safe.On success returns raw data; else bubbles or emits generic error. Good consolidation.
contracts/utils/LowLevelCall.sol (4)
49-70: staticcall variants mirror call variants accurately.Use of scratch space and memory-safe assembly looks correct.
95-110: returnData: correct Solidity bytes allocation and FMP update.Implementation addresses earlier safety concerns. LGTM.
112-125: bubbleRevert overloads: minimal and safe.Copying to FMP for last-call bubbling and reverting from provided buffer are both correct.
12-22: Assembly call sites: clean and gas-optimal.No unnecessary memory writes; outputs sized appropriately (0 or 0x40). Good use of "memory-safe".
Also applies to: 72-77, 79-93
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/modules/ROOT/pages/utilities.adoc (1)
497-497: Fix invisible non‑breaking space in the sample (copy/paste hazard).Prior review already flagged a U+00A0 between
||andLowLevelCall. Please ensure it’s an ASCII space.- if (!success || LowLevelCall.returnDataSize() < 32) { + if (!success || LowLevelCall.returnDataSize() < 32) {Run to scan the repo for NBSPs in docs and Solidity snippets:
#!/bin/bash # Find non-breaking spaces (U+00A0) across docs and Solidity files rg -nU --encoding utf-8 $'\xC2\xA0' --type-add 'adoc:*.adoc' -tadoc -tsol
🧹 Nitpick comments (4)
docs/modules/ROOT/pages/utilities.adoc (4)
313-316: Mark_hashFnaspure(no state read) and fix signature spacing.Hash helpers should be
pure; also standardizereturns (bytes32)spacing.-function _hashFn(bytes32 a, bytes32 b) internal view returns(bytes32) { +function _hashFn(bytes32 a, bytes32 b) internal pure returns (bytes32) {
464-489: Strengthen LowLevelCall example: mention static/delegate variants, avoid trailing-blank destructuring, and show revert bubbling.
- Add a sentence noting
staticcall/delegatecallvariants.- Avoid trailing-blank tuple by binding the unused value.
- Show how to bubble revert data on failure.
-=== Low-level Calls +=== Low-level Calls @@ -The xref:api:utils.adoc#LowLevelCall[`LowLevelCall`] library provides low-level external calls with fixed-size return data handling, protecting against return bombing attacks where callees allocate excessive memory. +The xref:api:utils.adoc#LowLevelCall[`LowLevelCall`] library provides low-level external calls with fixed-size return data handling, protecting against return bombing attacks where callees allocate excessive memory. It also includes `staticcall` and `delegatecall` variants with the same return-data handling and bubbling behavior. @@ using LowLevelCall for address; function example(address target, bytes memory data) internal { bool success; bytes32 result1; bytes32 result2; + bytes32 ignored; // Ignore return data success = target.callNoReturn(data); // Extract single 32-byte value - (success, result1, ) = target.callReturn64Bytes(data); + (success, result1, ignored) = target.callReturn64Bytes(data); + if (!success) { + LowLevelCall.bubbleRevert(); + } // Extract two 32-byte values (success, result1, result2) = target.callReturn64Bytes(data); + if (!success) { + LowLevelCall.bubbleRevert(); + } }
492-505: CachereturnDataSize()to avoid repeated calls and tighten the example.Minor gas/readability improvement: call once, reuse the value.
function checkReturnSize(address target, bytes memory data) internal returns (uint256 value, uint256 otherValue) { (bool success, bytes32 result1, bytes32 result2) = target.callReturn64Bytes(data); - if (!success || LowLevelCall.returnDataSize() < 32) { + uint256 size = LowLevelCall.returnDataSize(); + if (!success || size < 32) { return (0, 0); - } else if (LowLevelCall.returnDataSize() < 64) { + } else if (size < 64) { return (uint256(result1), 0); } else { return (uint256(result1), uint256(result2)); } }
564-618: Narrowusingscope for clarity in Time example.Bind methods only to
Time.Delayrather than*. Keeps extensions precise.- using Time for *; + using Time for Time.Delay;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/modules/ROOT/pages/utilities.adoc(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests-upgradeable
- GitHub Check: slither
- GitHub Check: coverage
- GitHub Check: tests-foundry
- GitHub Check: tests
- GitHub Check: halmos
🔇 Additional comments (1)
docs/modules/ROOT/pages/utilities.adoc (1)
76-76: LGTM on RSA paragraph tweak.Clarity/readability improved without changing meaning.
Alternative to #5091
Fixes #5013
Requires #5189
PR Checklist
npx changeset add)Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores